Upgrade to zarrita 0.7.2#398
Conversation
manzt
requested review from
ShrimpCryptid and
interim17
and removed request for
a team
|
Thank you for the PR! We will look at it asap... |
|
Having a look a this again today. I think there are some newer type-inference features used by the extension API that your TypeScript (v4.7) version doesn't support, so the inferred return types are Would you be open to me making a PR to upgrade TypeScript first, and then we can rebase on that? |
Sure. |
Zarrita 0.7 dropped the `Store` type generic on `zarr.Array` in favor of a composable store/array extension API. An audit of public repos turned up vole-core as the one consumer using the generic for something other than passing down an `AbortSignal`. I'm sorry for breaking your code. The previous code threaded `subscriber` / `reportChunk` / `isPrefetch` through the store to drive the cache and request queue. This can now be done by wrapping the array with a per-request `zarr.defineArrayExtension` that captures the same context in its closure. These changes follow the zarrita migration guide (https://zarrita.dev/migration/v0.7.html) to port to that extension model, move `RelaxedFetchStore`'s 403-handling onto `FetchStore`'s new `fetch` handler option, and swap `NodeNotFoundError` / `KeyError` for the consolidated `NotFoundError`.
|
@manzt should we do anything here? If we just fixup the conflicts is there anything else to be modified? |
manzt
force-pushed
the
manzt/zarrita-upgrade
branch
from
7d56370 to
997f1d3
Compare
|
I just rebased on main, fixed up issues, and pushed. Thinking it's in a good state. |
ShrimpCryptid
left a comment
ShrimpCryptid
left a comment
There was a problem hiding this comment.
Couple quick comments!
| const cached = opts.cache?.get(fullKey); | ||
| if (cached && isChunk(cached)) { | ||
| return cached; | ||
| } |
There was a problem hiding this comment.
Not sure why this got renamed?
| coords: TCZYX<number>, | ||
| subscriber: SubscriberId | ||
| ): void { | ||
| const instrumented = withVoleInstrumentation(scaleLevel, { |
There was a problem hiding this comment.
Might consider renaming this wrapper to be more descriptive... I feel like instrumented as a verb is not super descriptive to me. wrappedRequest? @frasercl might have more thoughts on this than I do
3 participants
Zarrita 0.7.0 dropped the
Storetype generic onzarr.Arrayin favor of a composable store/array extension API. An audit of public repos using zarrita led me to find this project as the one consumer using the generic for something other than passing down anAbortSignal. I'm sorry for breaking your code.The previous code threaded
subscriber/reportChunk/isPrefetchthrough the store to drive the cache and request queue. This can now be done by wrapping the array with a per-requestzarr.defineArrayExtensionthat captures the same context in its closure.These changes follow the zarrita migration guide to port to that extension model, move
RelaxedFetchStore's 403-handling ontoFetchStore's newfetchhandler option, and swapNodeNotFoundError/KeyErrorfor the consolidatedNotFoundError.